Speedups: remove dependency on c++ #2796
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
black-sliver
force-pushed
the
speedups-no-cpp
branch
from
941d835 to
809ad6a
Compare
ScipioWright
added
is: enhancement
Berserker66
reviewed
Feb 6, 2024
|
Here are some numbers for the case where len() is way bigger than 1023 from my machine: I'm not sure why std::set is that much slower to be honest, but that's just there for reference. So, the 1023 set is "half as fast", but it uses only ~230KB RAM compared to ~800KB for the "full" one. |
beauxq
reviewed
Feb 10, 2024
|
I played around with |
agilbert1412
added
the
waiting-on: peer-review
Berserker66
approved these changes
Feb 15, 2024
|
I ran ubsan on my local test code and nothing popped up. A thing to note is that the struct with the variable sized array is not valid C++, but it is valid C and supported by extensions to C++ in both GCC and clang (and probably msvc, but I haven't checked). I am still confused about the memset not working for Either way, I'd still like @beauxq to give feedback on the changed realloc before hitting merge. |
|
yeah, |
original idea was to only use bucket->val if int<pointer, but we always have a union now anyway
This kind of tests that the build succeeds.
…rnings Unnamed unions are not in C99, this got fixed. The overhead of setting count=0 is minimal or optimized-out and silences -fanalizer (see comment).
This won't happen in our cython, but it's still a good addition.
We solve this with env vars in ctest now, and this fails for msvc.
INTxx_MIN % 1 would not produce a negative number, so the test was flawed.
black-sliver
force-pushed
the
speedups-no-cpp
branch
from
cde9836 to
19109cc
Compare
Berserker66
approved these changes
Jun 12, 2024
agilbert1412
pushed a commit
to agilbert1412/Archipelago
that referenced
this pull request
Jun 13, 2024
* Speedups: remove dependency on c++ * Speedups: intset: handle malloc failing * Speedups: intset: fix corner case for int64 on 32bit systems original idea was to only use bucket->val if int<pointer, but we always have a union now anyway * Speedups: add size comment to player_set bucket configuration * test: more tests for LocationStore.find_item * test: require _speedups in CI This kind of tests that the build succeeds. * test: even more tests for LocationStore.find_item * Speedups: intset uniform comment style * Speedups: intset: avoid memory leak when realloc fails * Speedups: intset: make `gcc -pedantic -std=c99 -fanalyzer` without warnings Unnamed unions are not in C99, this got fixed. The overhead of setting count=0 is minimal or optimized-out and silences -fanalizer (see comment). * Speedups: don't leak memory in case of exception * Speedups: intset: validate alloc and free This won't happen in our cython, but it's still a good addition. * CI: add test framework for C/C++ code * CI: ctest: fix cwd * Speedups: intset: ignore msvc warning * Tests: intset: revert attempt at no-asan We solve this with env vars in ctest now, and this fails for msvc. * Test: cpp: docs: fix typo * Test: cpp: docs: fix another typo * Test: intset: proper bucket count for Negative test INTxx_MIN % 1 would not produce a negative number, so the test was flawed.
jnschurig
pushed a commit
to Tranquilite0/Archipelago-SoulBlazer
that referenced
this pull request
Jun 13, 2024
* Speedups: remove dependency on c++ * Speedups: intset: handle malloc failing * Speedups: intset: fix corner case for int64 on 32bit systems original idea was to only use bucket->val if int<pointer, but we always have a union now anyway * Speedups: add size comment to player_set bucket configuration * test: more tests for LocationStore.find_item * test: require _speedups in CI This kind of tests that the build succeeds. * test: even more tests for LocationStore.find_item * Speedups: intset uniform comment style * Speedups: intset: avoid memory leak when realloc fails * Speedups: intset: make `gcc -pedantic -std=c99 -fanalyzer` without warnings Unnamed unions are not in C99, this got fixed. The overhead of setting count=0 is minimal or optimized-out and silences -fanalizer (see comment). * Speedups: don't leak memory in case of exception * Speedups: intset: validate alloc and free This won't happen in our cython, but it's still a good addition. * CI: add test framework for C/C++ code * CI: ctest: fix cwd * Speedups: intset: ignore msvc warning * Tests: intset: revert attempt at no-asan We solve this with env vars in ctest now, and this fails for msvc. * Test: cpp: docs: fix typo * Test: cpp: docs: fix another typo * Test: intset: proper bucket count for Negative test INTxx_MIN % 1 would not produce a negative number, so the test was flawed.
sflavelle
pushed a commit
to sflavelle/Archipelago-tgc
that referenced
this pull request
Jun 20, 2024
* Speedups: remove dependency on c++ * Speedups: intset: handle malloc failing * Speedups: intset: fix corner case for int64 on 32bit systems original idea was to only use bucket->val if int<pointer, but we always have a union now anyway * Speedups: add size comment to player_set bucket configuration * test: more tests for LocationStore.find_item * test: require _speedups in CI This kind of tests that the build succeeds. * test: even more tests for LocationStore.find_item * Speedups: intset uniform comment style * Speedups: intset: avoid memory leak when realloc fails * Speedups: intset: make `gcc -pedantic -std=c99 -fanalyzer` without warnings Unnamed unions are not in C99, this got fixed. The overhead of setting count=0 is minimal or optimized-out and silences -fanalizer (see comment). * Speedups: don't leak memory in case of exception * Speedups: intset: validate alloc and free This won't happen in our cython, but it's still a good addition. * CI: add test framework for C/C++ code * CI: ctest: fix cwd * Speedups: intset: ignore msvc warning * Tests: intset: revert attempt at no-asan We solve this with env vars in ctest now, and this fails for msvc. * Test: cpp: docs: fix typo * Test: cpp: docs: fix another typo * Test: intset: proper bucket count for Negative test INTxx_MIN % 1 would not produce a negative number, so the test was flawed.
qwint
pushed a commit
to qwint/Archipelago
that referenced
this pull request
Jun 24, 2024
* Speedups: remove dependency on c++ * Speedups: intset: handle malloc failing * Speedups: intset: fix corner case for int64 on 32bit systems original idea was to only use bucket->val if int<pointer, but we always have a union now anyway * Speedups: add size comment to player_set bucket configuration * test: more tests for LocationStore.find_item * test: require _speedups in CI This kind of tests that the build succeeds. * test: even more tests for LocationStore.find_item * Speedups: intset uniform comment style * Speedups: intset: avoid memory leak when realloc fails * Speedups: intset: make `gcc -pedantic -std=c99 -fanalyzer` without warnings Unnamed unions are not in C99, this got fixed. The overhead of setting count=0 is minimal or optimized-out and silences -fanalizer (see comment). * Speedups: don't leak memory in case of exception * Speedups: intset: validate alloc and free This won't happen in our cython, but it's still a good addition. * CI: add test framework for C/C++ code * CI: ctest: fix cwd * Speedups: intset: ignore msvc warning * Tests: intset: revert attempt at no-asan We solve this with env vars in ctest now, and this fails for msvc. * Test: cpp: docs: fix typo * Test: cpp: docs: fix another typo * Test: intset: proper bucket count for Negative test INTxx_MIN % 1 would not produce a negative number, so the test was flawed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What is this fixing or adding?
Removes dependency on C++ for _speedups.
But Why? C++ adds extra dependencies that need to be loaded into the process and afaik only _speedup requires them at the moment. Also currently we also don't handle C++ exceptions that could occur. This change fixes both and saves some memory because it should always have less memory overhead.
How was this tested?
Only pytest and a few benchmarks and tests on C level. I wanted to push this so we can discuss.